Abort failing FindReplaceDocumentAdapter operations consistently #2657 #2664
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
HeikoKlare
commented
Jan 6, 2025
| Matcher replaceTextMatcher= pattern.matcher(prevMatch); | ||
| replaceText= replaceTextMatcher.replaceFirst(replaceText); | ||
| } catch (IndexOutOfBoundsException ex) { | ||
| } catch (IndexOutOfBoundsException | IllegalArgumentException ex) { |
Contributor
Author
There was a problem hiding this comment.
This is necessary to fulfill the intention of interpretReplaceEscapes() to leave an unfinished character escape at the end of the pattern as is (such as in the pattern demo\) in order to produce a proper error message (which is delivered through an IllegalArgumentException from Matcher#replaceFirst()).
Contributor
There was a problem hiding this comment.
such comment should be put in the code
Contributor
Author
There was a problem hiding this comment.
Indeed, I have added a comment to the code explaining why these exceptions are caught in f020fbe.
Contributor
|
This pull request changes some projects for the first time in this development cycle. An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch. Git patchFurther information are available in Common Build Issues - Missing version increments. |
Contributor
…pse-platform#2657 The implementation of FindReplaceDocumentAdapter#findReplace(), used by its API methods #find() and #replace(), currently leaves an inconsistent state when failing because of an invalid input or state. This inconsistent state affects the last executed operation type, which the adapter stores in order to identify whether a replace operation is preceded by an according find operation. In case the #findReplace() method is called with an invalid position or to execute a replace without a preceding find, the method aborts (throwing an exception) without storing the operation to be executed as the last executed operation, i.e., it leaves the adapter as if that method was never called. In case the method is called in regular expression mode and the expression used as find or replace input is invalid, the operation aborts (throwing an exception) but still stores the operation to be executed as the last executed operation, i.e., it leaves the adapter as if that method was called successfully. This behavior is unexpected as is handles invalid inputs inconsistently. This also becomes visible in the existing consumers, such as FindReplaceTarget#replaceSelection() used by the FindReplaceDialog and FindReplaceOvleray. They assume that in case an exception occurs while trying to perform a replace operation, a subsequent replace should succeed without an additional find operation being required. This assumption does currently not hold. This change corrects the behavior of the FindReplaceDocumentAdapter#findReplace() method to always leave the adapter in an unmodified state when the method fails because of being called with invalid input or in an inconsistent state. In addition, regular expressions with an unfinished character escape at the end now lead to a proper exception. Fixes #eclipse-platform#2657
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The implementation of FindReplaceDocumentAdapter#findReplace(), used by its API methods #find() and #replace(), currently leaves an inconsistent state when failing because of an invalid input or state. This inconsistent state affects the last executed operation type, which the adapter stores in order to identify whether a replace operation is preceded by an according find operation. In case the #findReplace() method is called with an invalid position or to execute a replace without a preceding find, the method aborts (throwing an exception) without storing the operation to be executed as the last executed operation, i.e., it leaves the adapter as if that method was never called. In case the method is called in regular expression mode and the expression used as find or replace input is invalid, the operation aborts (throwing an exception) but still stores the operation to be executed as the last executed operation, i.e., it leaves the adapter as if that method was called successfully.
This behavior is unexpected as is handles invalid inputs inconsistently. This also becomes visible in the existing consumers, such as FindReplaceTarget#replaceSelection() used by the FindReplaceDialog and FindReplaceOvleray. They assume that in case an exception occurs while trying to perform a replace operation, a subsequent replace should succeed without an additional find operation being required. This assumption does currently not hold.
This change corrects the behavior of the FindReplaceDocumentAdapter#findReplace() method to always leave the adapter in an unmodified state when the method fails because of being called with invalid input or in an inconsistent state. In addition, regular expressions with an unfinished character escape at the end now lead to a proper exception.
Fixes #2657
How it behaves
This is how it behaves after the fix
The inconsistent error indication (input colored red) is specific to the FindReplaceOverlay and will be fixed in a follow-up PR.